-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Center videos correctly & fix padding for controls #4255
Conversation
@blackbox87 this fix also this #4179 ? |
1005305
to
0bd2130
Compare
@domiuns On my device it only takes me 1 tap to see both the status bar and the video controls and this commit does nothing to change that. |
0bd2130
to
6b27bce
Compare
Sorry about the forced pushes. I messed up and needed to correct things.
Basically it's difficult to accurately detect if you've got a cutout, it's position and it's size. So without the padding you might end up having the controls or the videos title placed under a cutout. That's why padding is now being applied to all devices as a precaution. I've tested the changes on my old phone without a cutout and my new phone with a cutout. It looks good to me. |
Could upload a test apk so other people can test it? |
@blackbox87: Can't you use https://developer.android.com/reference/android/view/WindowInsets#getDisplayCutout()? Btw force pushing isn't a problem for PRs, we like having a clean (but not "too clean") commit history. Also, do you post, then delete and repost a comment? I get multiple notifications from your comments, but if that's the case, could you please just edit your comment? |
@opusforlife2 Done. @wb9688 Not easily, no. Mainly because you want the controls to always be centered on the screen, but when using that API you'll need to calculate things to center the controls and it needs to change as you rotate your device. I opted to do it the easier way, which is to add enough padding to the left and right sides of the screen so that the controls are centered. And although it's possible not to apply that extra padding on devices without a cutout I haven't done that for the sake of consistency.
I don't believe I have, at least not deliberately. |
@blackbox87 I thought extra padding will look bad but actually it is quite nice with your code. I didn't tested everything, just launched the app to see the changes of look. The only thing I want to ask you is to remove extra padding for devices without cutout and here is why. I don't want to pay for someone's ugly displays. Yes, I hate these big terrible cutouts and never liked it. You or someone else may disagree but if you like it, not a problem, just don't add extra padding for those who has displays without cutouts. Big thank you that you found a way to work around this big Android API problem. |
Found an issue while testing. You'll see that padding on top and bottom is removed, so controls located under navigation bar, and status bar, and cutout. Let me know if I need to make a screenrecord. |
Please do as I'm probably doing something wrong, but I can't replicate that issue. Seeing it would help. I'm probably probably going to adjust the code slightly anyway since I just tested it on Android 4.4 emulator and I don't like how the black navbar makes the controls look off center, even though they are centered. |
You mean "you should see me in a crown" :P |
@avently |
Better to avoid it because you can call this in the wrong place (while video is rotating, for example). And the resulting values for screen elements will be wrong. setControlsSize() is kind of magic. I do here what Android should do for us, so it's hard to say to you the correct place when the code should be changed and how. But the main rule is to avoid calling this method many times because it can be called in many device's UI states |
Maybe this is a problem of the issue:
Here you don't think about situation when a video can be in fullscreen but not in landscape. But it's looking at the code on github, didn't tested. |
Looks like some day we will end up by removing all this custom code in favor of UI without status bar and navigation bar that's simple but not so useful:) But for now let's do cool custom things:) |
I have no cutouts. In a vertical video (we all know which one :P ) the bottom stuff (seekbar etc.) is padded, but the top stuff isn't. But I agree with avently that devices without cutouts shouldn't be affected by this PR at all. |
noooo @avently, anything but the status bar. like you said, let's stay cool 😎 and useful. 😊 |
Nice, this apk crash and block my device in loop because when i open an video app crash and phone try to reopen video. I have remove battery for unlock phone |
@domiuns None of my changes should cause that, so maybe the issue is with the current dev branch. Instead of removing your battery you can probably hold your power button down for 10-20 seconds. |
@blackbox87 demostration when i click on video https://streamable.com/e/ygkbwt |
@domiuns Try the 2020-09-08 version posted here > https:/TeamNewPipe/NewPipe/projects/17#card-42918515 Does it have the same issue? @avently I fixed the bug that you reported, but now I'm stuck trying to detect cutouts correctly. Your hasCutout() method doesn't work because it assumes that the status bar height will be adjusted for a cutout, which isn't the case on my device. So I've tried using getDisplayCutout() instead, but if I start NewPipe in landscape then it'll often return null. |
@blackbox87 can't help you in this case because never did something with cutouts in my apps. If you will not find a solution, not a problem, will leave as the current state of dev branch. Not a big deal I think to have a non-centered vertical video and masked cutout space in landscape. |
I think it is a problem since it worked perfectly before the unified player was added. That's the only reason I'm still trying to fix this. At the moment I can't say that I'm a fan of the unified player as it's introduced more issues than it solves. |
@blackbox87 cool! Thank you. So you're ready to upload the latest changes and to merge the PR? Or something is not ready yet? |
If everything looks good and nobody can find any issues then I'll update this PR and then it should be ready for merging. I'm currently waiting to see if my latest changes also fix #4277. |
Based on my testing, all works perfectly:
So, yeah, works great. Great job, man! |
But note that I use a phone with 16:9 aspect ratio, on other screens situation maybe different |
4e90c5f
to
a5e125e
Compare
a5e125e
to
ffedf13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for the effort!
@@ -203,6 +207,11 @@ | |||
private int cachedDuration; | |||
private String cachedDurationString; | |||
|
|||
private boolean gotCutout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private boolean gotCutout; | |
private boolean hasCutout; |
if (rotation == Surface.ROTATION_90) { | ||
if (gotCutout) { | ||
ctrlPadLeft = controlsPadding + insetLeft; | ||
ctrlPadRight = controlsPadding | ||
+ (getNavBarMode() == 2 ? 0 : navBarHeight) + insetRight; | ||
} else { | ||
ctrlPadLeft = controlsPadding; | ||
ctrlPadRight = controlsPadding; | ||
} | ||
} else { | ||
if (gotCutout) { | ||
ctrlPadLeft = controlsPadding | ||
+ (getNavBarMode() == 2 ? 0 : navBarHeight) + insetLeft; | ||
ctrlPadRight = controlsPadding + insetRight; | ||
} else { | ||
ctrlPadLeft = controlsPadding; | ||
ctrlPadRight = controlsPadding; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you only check for 90 deg, but not for 180?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TobiGr When it's 90° in landscape then the navbar is on the right side of your screen, so the else statement handles the opposite. And this orientation check only applies to phones in landscape, so it always seems to work.
If you look at #4272 then you'll see that avently worked on a cleaner method, but we had a disagreement about handling a navbar feature that's found on a lot of custom Android 10 ROMs (LineageOS, ArrowOS, Resurrection Remix, PixelExperience, AICP etc).
@TobiGr there is more recent method of doing the same thing as this PR but in more correct/universal way: #4272 (comment) This PR contains one more thing (useful for @blackbox87) that can be ported over my PR: more padding for devices with hidden navbar and enabled gestures on custom ROMs. We have a talk related to all this in the link I wrote. |
@TobiGr @Stypox I'm closing this PR because #4272 handles the insets in a cleaner way, although I still think the overlapped gesture navbar issue should be handled by #4272 rather than a separate PR. And I'm sure Avently will disagree, but the only reason I opened this PR was because he didn't want to fix the issue until I showed that it could still be done. So originally I had to base my changes on top of his, then rebase to the dev branch and then Avently looked at my changes and bundled his own version of the cutout fix into a PR that fixes multiple different issues. I won't submit another PR with a fix for hidden gesture navbars, but if someone else wants to do it then here's a fix that's been tested against a few Android 10 ROMs. |
i can confirm this works with a galaxy s10+ with modpunk Linage os. |
#4272 doesn't fix the black notification bar problem. |
@blackbox87 thank you for your contribution! Without it I wouldn't start thinking about cleaner and better way of positioning the player. Your PR is really important for me because there are a few people who really do PRs here. And I was surprised that you started to to so. Even if we disagree with each other in some situations I think we did a great job and can do it in the future too. |
@fareszr Black notification bar problem? I'm not sure which that is, but I'd guess that once #3178, #4272 and #4288 are merged then it'll be fixed. You could try using this or this? But you'll probably want to continue that discussion in #4272 if the issue occurs because of one of avently's commits. |
|
@fareszr What's your phone model? If your device has hardware buttons then how that looks might actually be correct, but if you've got the navbar hidden via a setting then that's likely the issue as Avently's version doesn't check the navbar heights or modes. If I use Avently's version on my device running LineageOS 16 it looks correct. |
i chose the zoom option to fill the screen. device: S10+ with modpunk's Los 17.1 |
I don't need it because Android adjusts view bounds for me. On screenshot it looks like he should see nav bar but it is invisible (or only layout was adjusted without showing buttons from SystemUI which is strange because others don't have such problem). @fareszr
And lets move to #4272 since that pr is a better place for such discussion |
@fareszr it's a known bug of your ROM Known bugs ->
So, will not be fixed, ask your rom developer |
@fareszr Oops, sorry. I see that you mentioned the phone model in your original post. @avently Good catch. And I agree with not trying to fix bugs that are caused by unofficial ROMs. I'm not entirely sure why my version still works though, unless it has something to do with the layout changes you've made (adding/removing fitsSystemWindows?). Everything else should be nearly the same, since we both only apply padding to the controls. |
In your code you still have FLAG_LAYOUT_NO_LIMITS because you arrange the views manually. I don't do that so I don't have such flag and I let the system do all adjustments. His rom doesn't apply correct adjustments if flag FULLSCREEN is set |
@avently Ah! Of course, that'll do it. I do agree with not fixing the issue in this case. I still don't agree on the other issue though, since almost every Android 10 and 11 ROM that isn't based on pure AOSP has the option to hide the gesture bar. But if someone wants to fix it then I've shared a little code that does the trick. If nobody fixes the issue with the hidden gesture bar within a few months then I'll consider making the PR myself. At the moment I just don't have a lot of free time and personally I'd rather wait for commits to the unified player to slow down so that there's less conflicts. |
well thats unfortunate i am stuck with non-unified player now, all other apps work correctly including youtube its only an issue with newpipe debug edition and avently PR. |
@fareszr that's interesting, so current release works ok for you?
|
oh wow its fixed in the final2.zip file, i downloaded the initial test app from the main PR comment. |
Glad there is no issue to fix:). Testing apk is a thing that changes often so when you test something from a PR take a look at the end of the page. The truth is always at the end:) |
What is it?
Description of the changes in your PR
Fixes the following issue(s)
It improves on #4154 and fully fixes #4040
Testing apk
app-debug.zip
Agreement